Skip to content

Conversation

@lotrien
Copy link
Member

@lotrien lotrien commented Jan 1, 2018

In scope of this pr basic functionality for snippet posting were implemented.

@lotrien lotrien requested review from ikalnytskyi and malor January 1, 2018 22:28
.eslintrc.json Outdated
}],
"react/prop-types": [0]
"react/prop-types": [0],
"jsx-a11y/no-noninteractive-element-interactions": [0],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I do understand why jsx-a11y/click-events-have-key-events is needed. But why do we need this one? There's no rationale in the commit message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our case we sometimes use click events on li, div, ul, etc elements which are not called non-interactive-elements. Since we can't move this events to other elements and we have this eslint rule enabled we will have an error. But with this rule disabled its possible for us to use such events

export const postSnippet = snippet => dispatch => (
fetch('http://api.xsnippet.org/snippets', {
method: 'POST',
mode: 'cors',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken, cors is default value. I'd suggest to do not mention this, as we don't set it explicitly for other requests we make.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will remove

method: 'POST',
mode: 'cors',
headers: {
Accept: 'application/json',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use quotes here? Just to be consistent with 'Content-Type'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our lint will throw an error if I will use quotes here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️

Copy link
Member Author

@lotrien lotrien Jan 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can add rule to always use quotes for values if this will help feel better

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, but not this time.


onInputChange(e) {
const { name, value } = e.target;
const result = (name === 'tags')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Geez.. I'm not insist, but using plain if would be more readable (and readability matters!).

onInputChange(e) {
  const { name, value } = e.target;

  if (name === 'tags') {
    value = value.split(',').map(item => item.trim())
  }

  this.setState({ [name]: value })
}

Copy link
Member Author

@lotrien lotrien Jan 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will change, but not exactly like you proposed, since value variable already defined this will occur an error

? value.split(',').map(item => item.trim())
: value;

this.setState({ [name]: result });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why name is wrapped in brackets (i.e. [name])?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to reuse this function for a few similar inputs, and I want to set their values in state, so I will have: {tags: some_value} and {title: some_other_value}.
If I will use name without [] it will think that I want this {name: some_value}.
In this case js syntax lets me to use name as different key

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, gosh, Jesus.. so basically if you use name without brackets, it will think that name is a name of key?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeap, exactly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️ what a silly language

}

postSnippet(e) {
e.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add few lines with explanations why preventDefault() is necessary. If I'm not mistaken it's because we don't want to follow default HTML behavior when form data is submitted to specified URI using POST and multipart/form-data content type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm.It seems obvious to me

}

onSyntaxClick(syntax) {
this.setState({ syntax }); // eslint-disable-line react/no-unused-state
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this ignore rule in two places?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because our lint checker creates to error messages for those two lines

onClickHandler(e) {
const { index, syntax } = e.target.dataset;

this.setState({ activeItem: { [index]: true } });
Copy link
Member

@ikalnytskyi ikalnytskyi Jan 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have few questions here:

  • Why do we need to wrap index in brackets (i.e. [index])? What does it mean? Why just not index?
  • Why does activeItem is an object with just one bool which can't be even false? I'd suggest to use the following schema:
    this.setState({ activeIndex: index })
    What do you think?
  • It seems like index is not read from state, so why define one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. look on one of answers above about these [] brackets;
  2. and 3. Take a look on line 39 where I check is newly selected item has appropriate index, in this way I can avoid additional loops that will do nothing more than remove active class from previous item

@lotrien lotrien force-pushed the lang-bar branch 3 times, most recently from 544537b to 9a8e3cb Compare January 2, 2018 19:19
since we find this not actual for our project, due to appropriate work
onClick events on different keypress events.
In scope of this pr basic functionality for snippet posting were
implemented.
@ikalnytskyi ikalnytskyi merged commit 4c2bd61 into master Jan 2, 2018
@ikalnytskyi ikalnytskyi deleted the lang-bar branch January 2, 2018 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants